ci: add a manual firestore flake probe for #776 - #780
Conversation
Every measurement of the FirebaseExtended#776 flake so far has been local, where the failure is a plain waitFor timeout with no gRPC error. In CI it arrives alongside a gRPC framing desync (RESOURCE_EXHAUSTED: Received message larger than max), which raises the possibility that the local repro and the CI failure are not the same bug. That matters, because the @grpc/grpc-js override proposed as the fix was measured only against the local one. This runs the firestore suite N times per arm, across both Node versions and both grpc-js versions, under CI conditions, so the comparison happens where the failure actually occurs. Notes on the design: - workflow_dispatch only. It never runs on a push, a PR or a schedule, so it costs nothing until someone asks for it. - A fresh emulator per iteration, matching how npm test runs in CI. Reusing one emulator across iterations would measure something else. - Failures are classified, not counted. Only the FirebaseExtended#776 assertion signature counts toward the rate; emulator start failures are reported separately, because folding them in previously inflated a local rate estimate by roughly 50%. - The job reports rather than fails. A red run here means the probe broke, not that the flake reproduced. - Inputs reach the script through env rather than interpolation, and iterations is validated before it reaches the loop. Classifier dry-run against synthetic logs covering pass, flake, flake-with-gRPC-error and infra-failure returns the expected counts and excludes infra failures from the rate. zizmor 1.25.2 reports no findings beyond the cache-poisoning rule CI suppresses. Refs FirebaseExtended#776
armando-navarro
left a comment
armando-navarro
left a comment
There was a problem hiding this comment.
Nice, this is worth having. Approving it. A few things I would think about before anyone runs it and reads numbers off the table.
The two arms land on different runners
arm is a matrix dimension, so baseline and override come out as separate jobs on separate machines. You were careful about this on your laptop, same machine and back to back, and that is the one control that did not make the trip. Given the whole premise here is that the machine matters, I would pull arm out of the matrix and loop both arms inside one job instead. Costs you a longer job, but then the comparison means something.
It is not running the workload the failures came from
CI does npm run test, so five emulators, the whole suite, parallel workers. This does one emulator and one file.
Your local runs were firestore-only too and still hit 6/44, so I am not saying it will fail to reproduce anything. But RESOURCE_EXHAUSTED has never turned up in a firestore-only run, and that is the signal you actually care about. What worries me is that you reproduce the laptop bug on CI hardware and the CI bug never shows up. Might be worth a line in the job summary so nobody reads a clean table as a verdict.
The RESOURCE_EXHAUSTED number is a total, not an overlap
grpc_err goes up before the run gets classified, so Flake: 2 sitting next to RESOURCE_EXHAUSTED: 11 does not tell you whether those 2 are inside the 11. That overlap is the whole question. One more counter in the flake branch and it is in the table.
Hangs fall out of the rate entirely
If Firestore calls never finish you get Error: Test timed out in 5000ms and no assertion line, so the run goes to infra and drops out of both halves of the rate. #776 mentions a 120s hang. Should those get their own bucket?
Related: the flake match is a literal assertion string, so if vitest ever rewords it, every real flake quietly becomes an infra failure.
On run counts
#776 calls 30 per arm underpowered at p = 0.12, and the default here is 20.
Separately, the guard lets you pass 200. At your 25s estimate that is 83 minutes against a 60 minute timeout. I clocked 7s locally so your estimate is probably pessimistic, but if it ever does hit the wall the summary block never runs and you lose every count.
Your confound is not actually there
I diffed the two lockfiles and exactly one version moves: @grpc/grpc-js, 1.9.16 to 1.14.4. google-gax was already on 1.14.4, so its nested copy just dedupes into the hoisted one. Nothing else budges. The arms differ by the Firestore client's grpc-js and nothing else, which is cleaner than your caveat makes it sound.
One last thing. I checked the classifier by actually breaking the test rather than trusting the grep on sight. Shortened the waitFor on returns undefined if document does not exist and ran your command against it. Your pattern catches it.
Six fixes from Armando's review on FirebaseExtended#780: - Pull `arm` out of the matrix. Both arms now run sequentially in one job on one runner, so the machine is held constant. That was the control the local measurements had and the workflow dropped, and the premise of the probe is that the machine matters. Arm order is forced baseline-then-override because applying the override mutates node_modules for everything after it. - Note in the job summary that this runs one emulator and one file, while CI runs five emulators and the whole suite in parallel, so a clean table is not a verdict on CI. - Count the flake/RESOURCE_EXHAUSTED overlap. Two independent totals could not answer whether the gRPC desync and the FirebaseExtended#776 assertion co-occur, which is the question the probe exists for. - Give hangs their own bucket. A test timeout produces no assertion line, so it was landing in `infra` and dropping out of the rate entirely. FirebaseExtended#776 reports a 120s hang. - Persist counts per arm and render the summary in an `always()` step. A run that hits `timeout-minutes` now still reports the arms that finished, instead of losing every count. Default iterations 20 -> 30, matching FirebaseExtended#776's power note. - Surface unrecognized failures. The flake match is a literal vitest assertion string, so a reword would have quietly turned every real flake into an infra failure. Unmatched assertion lines are now collected and shown as a warning in the summary. Also drops a `set +e` / `set -e` pair that turned errexit on partway through a script that never had it enabled. Verified: classifier dry-run over synthetic logs covering pass, flake, flake-with-gRPC, hang, infra and a reworded assertion returns the expected counts and routes the reword to the unmatched warning; arms input validation rejects empty, unknown and non-JSON values; the summary renders correctly from a partial counts file, which is the timeout-recovery path; zizmor 1.25.2 reports no findings beyond the suppressed cache-poisoning rule.
|
Thanks, all six are in as The two arms landing on different runners. You're right and this is the one that mattered most: the premise is that the machine matters, so splitting the arms across runners reintroduces the variable being measured. Not running the workload the failures came from. Kept the scope, took the line in the summary. Every run now ends with a footer saying this is one emulator and one file against CI's five emulators and parallel suite, that The RESOURCE_EXHAUSTED total vs the overlap. Added a Hangs falling out of the rate. They get their own bucket now, matching Run counts. Default is 30 now. On the 200 guard: rather than lower the cap, counts are appended per arm to a The confound. Thanks for diffing the lockfiles, that is better than the caveat I wrote. I'll narrow the "Known confound" section in the body to what you found: only Also dropped a Re-verified after the changes rather than assuming: classifier dry-run over synthetic logs covering pass, flake, flake-with-gRPC, hang, infra and the reworded assertion returns the expected counts; the arms input rejects empty, unknown and non-JSON values; zizmor 1.25.2 still reports nothing beyond the suppressed And noted on checking the classifier by actually breaking the test rather than reading the grep. That's the second time this week your verification caught something mine wouldn't have. |
2 participants
Refs #776. This measures the flake; it does not fix it.
Why
Every measurement of the #776 flake so far has been on a laptop. Locally the failure is a plain
waitFortimeout with no gRPC error at all. In CI it arrives alongside a gRPC framing desync:A ~795 MB claimed frame against a 4 MB cap means the reader is interpreting payload bytes as a length header. That is a
@grpc/grpc-js-layer failure, and it does not appear in any local run.So the local repro and the CI failure may not be the same bug. That matters, because the
@grpc/grpc-jsoverride proposed as the fix was measured only against the local one, at p = 0.12. Merging that override would also mean CI never runs 1.9.16 again, so it is a before-and-after with no control.This workflow makes the comparison in CI, where the failure actually happens, before anything is changed.
What it does
Matrixes Node 22 and 24. Within each job it runs both
@grpc/grpc-jsarms sequentially, baseline then override, N times per arm (default 30), and writes a table per arm to the job summary: resolved grpc-js version, pass / flake / hang / infra counts, how many runs showedRESOURCE_EXHAUSTED, how many flakes showed it too, and the resulting rate.Design decisions worth reviewing:
workflow_dispatchonly. It never runs on a push, a PR, a release or a schedule, so it adds nothing to normal CI time.node_modulesfor everything after it.npm testruns in CI. Reusing one emulator across iterations would measure something else.infraand drop out of the rate entirely. CI: test/firestore.test.tsx flakes intermittently on both Node 22 and Node 24 #776 reports a 120s hang.always()step from counts persisted per arm, so a job killed bytimeout-minutesstill reports the arms that finished instead of losing every count.RESOURCE_EXHAUSTEDis counted per arm, and its overlap with the flake count is counted too. Two independent totals cannot say whether the gRPC desync and the CI: test/firestore.test.tsx flakes intermittently on both Node 22 and Node 24 #776 assertion co-occur, which is the question the probe exists for.envrather than being interpolated into the shell, and bothiterationsandarmsare validated before they reach the loop.RESOURCE_EXHAUSTEDhas never appeared in a firestore-only run, so this configuration may reproduce the local failure while never reaching the CI one. Every job summary repeats this, because a clean table here is not a verdict on CI.Verification
workflow_dispatchfor workflows present on the default branch, so it is unrunnable until this merges. Per the repo's own lesson about treating jobs authored on a branch as unrun, it was verified out of band instead:timeout-minutestakes.armsinput rejects empty, unknown and non-JSON values, and normalizes to baseline-first regardless of the order given.cache-poisoningrule CI suppresses, which is the severity set the gate reads.package.jsonoverride edit produces the right key (npm pkg setmangles keys containing a slash, hence thenode -e);npx firebaseresolves throughfirebase-tools.The first real run is therefore the first proof it works. If it misbehaves, the fix is another PR, not a rollback of anything consumers see.
Known confound: narrower than first stated
Corrected after review. This section originally warned that the repo-wide override moves
@grpc/grpc-jsforfirebase-toolsandgoogle-gaxas well as the Firestore client, so the arms differed by more than the library under test. A lockfile diff of the two arms shows that is not the case: exactly one version moves,@grpc/grpc-js1.9.16 to 1.14.4.google-gaxwas already on 1.14.4, so its nested copy simply dedupes into the hoisted one. Nothing else budges.So the arms differ by the Firestore client's grpc-js and nothing else, which is a cleaner comparison than the original caveat implied. Credit to @armando-navarro for diffing it rather than taking the caveat at face value.
What this changes for anyone else
Nothing. One new file, no existing workflow touched, no runtime or packaging change, and no effect on normal CI.